Add lambert azimuthal equal area coord_system by lbdreyer · Pull Request #1738 · SciTools/iris

lbdreyer · 2015-07-28T15:04:15Z

Depends on SciTools/iris-test-data#37 and SciTools/cartopy#619

rhattersley · 2015-08-04T09:24:22Z

lib/iris/coord_systems.py

Please could you add "Defaults to 0." for these arguments.

rhattersley · 2015-08-05T11:07:10Z

👍 Now it just needs a new Cartopy release...

lbdreyer · 2015-08-05T12:45:20Z

It probably also needs an entry to the what's new.

rhattersley · 2015-08-05T16:41:37Z

It probably also needs an entry to the what's new.

Yes, good point. 👍

lbdreyer · 2016-09-28T13:31:18Z

@bjlittle , this is the pull request I was talking about that is now needed.

lbdreyer · 2016-09-28T13:31:21Z

lib/iris/fileformats/netcdf.py

+                        (iris.coord_systems.TransverseMercator,
+                         iris.coord_systems.LambertAzimuthalEqualArea)):
+            units = 'm'
+


This block was removed in PR #1943 but I don't agree that it should have been removed.
Section 4.1 and 4.2 of the cf conventions says:

Coordinates of latitude with respect to a rotated pole should be given units of degrees

also 'm' is a perfectly fine unit (see the example in this section).

@lbdreyer Interesting that swapping this back doesn't cause any test failures ...

In fact, if you look at Example 5.6, that's a rotated_lat_lon grid with units of "degrees". Also Example 5.10 is a tranverse_mercator grid with units of "m".

Right, I think I must have gotten confused trying to rebase a PR that was over a year old!

On closer inspection I now see that, in the case of tranverse_mercator we're just passing the units from the coord. Although this has it's disadvantages (i.e. it means that a user could produce a netcdf file with a badly defined coordinate:
e.g. DimCoord(array([30, 60]), standard_name='latitude', units=Unit('m'), coord_system=RotatedGeogCS(10.0, 20.0))
But I would prefer for users to have this flexibility, and cf says that there is no default value for lat or lon units.

As the CF conventions recommend a specific unit for GeogCS and RotatedGeogCS I think you could argue that we should 'enforce' these:

The recommended unit of latitude is degrees_north

and

Coordinates of latitude with respect to a rotated pole should be given units of degrees

lbdreyer · 2016-09-28T13:52:54Z

Failing because of this:
ERROR: test_365_calendar (iris.tests.unit.fileformats.pp_rules.test_convert.TestLBTIM)
but I didn't touch that area of code 😕

ajdawson · 2016-09-28T13:59:03Z

That failure was fixed in #2144, targeted at the v1.10.x branch. This change needs to make its way onto master, and then you can rebase onto that to get passing tests here (or we can trust that this feature doesn't cause the failure). There is another test failure too (an error actually), not sure what that one is caused by.

lbdreyer · 2016-09-30T11:41:13Z

I am going to add a test for the coordinate units discussed in the above comment (I'm hoping that's not a controversial change), and then rebase.

lbdreyer · 2016-09-30T14:55:31Z

lib/iris/tests/unit/fileformats/netcdf/test_Saver.py

+    def check(self, coord_system, units, expected_units):
+        coord = iris.coords.DimCoord([30, 45], 'latitude', units=units,
+                                     coord_system=coord_system)
+        saver = Saver('filename', 'NETCDF4')


Argh! This is wrong. I was going to mock it out.

I had made _cf_coord_identity a static method in the hopes that I wouldn't have to instantiate the Saver class to test it, but I was getting an error

>>> coord = iris.coords.DimCoord([45], 'latitude', units='m') >>> r = Saver._cf_coord_identity(coord) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: unbound method _cf_coord_identity() must be called with Saver instance as first argument (got DimCoord instance instead)

lbdreyer · 2016-10-03T14:10:18Z

@bjlittle The tests are now passing and the static method is working correctly.

marqh · 2016-10-10T08:49:17Z

Hi @lbdreyer

i'm afraid something has occurred on master such that your branch now has conflicts

please rebase and fix so we can see a plausible change with test results

are you still waiting on a cartopy release for this to be able to work properly?

mark

lbdreyer · 2016-10-10T09:59:45Z

@marqh

I have rebased and the tests are passing again.

are you still waiting on a cartopy release for this to be able to work properly?

No, the change that this required was in the Cartopy 0.14 release

bjlittle

Some minor changes, and some feedback from @djkirkham and @marqh would be welcome regarding rotated pole and (CF spec) units of degrees

bjlittle · 2016-09-28T14:46:27Z

lib/iris/fileformats/netcdf.py

+                        (iris.coord_systems.TransverseMercator,
+                         iris.coord_systems.LambertAzimuthalEqualArea)):
+            units = 'm'
+


@lbdreyer Interesting that swapping this back doesn't cause any test failures ...

bjlittle · 2016-10-10T13:19:19Z

lib/iris/fileformats/netcdf.py


-        return (coord.standard_name, coord.long_name, units)
+        elif isinstance(coord.coord_system, iris.coord_systems.RotatedGeogCS):
+            units = 'degrees'


@lbdreyer As per the CF spec sections 4.1 and 4.2.

@djkirkham and @marqh This reverts your changes from #1943. Can we all agree this is what we want for rotated pole units, as per CF.

If I remember correctly, we took this out because the units may be radians, say, so changing to degrees would be incorrect.

Thanks @djkirkham, that makes sense. I will remove this.

bjlittle · 2016-10-10T13:24:29Z

lib/iris/tests/unit/coord_systems/test_LambertAzimuthalEqualArea.py

+        self.semi_major_axis = 6377563.396
+        self.semi_minor_axis = 6356256.909
+        self.ellipsoid = GeogCS(self.semi_major_axis, self.semi_minor_axis)
+        self.laea_cs = LambertAzimuthalEqualArea(


@lbdreyer Add in the false_easting and false_northing ... thanks!

bjlittle · 2016-10-10T13:29:36Z

lib/iris/tests/unit/fileformats/netcdf/test_Saver.py

+    def check_call(self, coord_system, units, expected_units):
+        coord = iris.coords.DimCoord([30, 45], 'latitude', units=units,
+                                     coord_system=coord_system)
+        result = Saver._cf_coord_identity(coord)


@lbdreyer Hence the need for @staticmethod, okay ... 😄

bjlittle · 2016-10-10T13:32:45Z

lib/iris/tests/unit/fileformats/netcdf/test_Saver.py

+class Test__cf_coord_identity(tests.IrisTest):
+    def check_call(self, coord_system, units, expected_units):
+        coord = iris.coords.DimCoord([30, 45], 'latitude', units=units,
+                                     coord_system=coord_system)


@lbdreyer Ideally, to give full coverage, could you also test longitude also ... thanks!

bjlittle · 2016-10-10T13:33:56Z

lib/iris/tests/unit/fileformats/netcdf/test_Saver.py

+        self.check_call(coord_system=crs, units='degrees',
+                        expected_units='degrees')
+
+    def test_crs_with_no_default_units(self):


@lbdreyer This is really a pass-thru rather than default behaviour ... what do you think?

bjlittle · 2016-10-10T13:35:27Z

lib/iris/tests/unit/fileformats/netcdf/test_Saver.py

+            latitude_of_projection_origin=52,
+            longitude_of_projection_origin=10,
+            false_easting=100,
+            false_northing=200)


@lbdreyer Did you want to pass-thru a choice of ellipsoid here ...

bjlittle · 2016-10-10T15:21:51Z

@lbdreyer Great work! Thanks 👍

lbdreyer force-pushed the lambert-equal branch from fe10770 to 3911e7f Compare July 29, 2015 13:26

rhattersley reviewed Aug 4, 2015
View reviewed changes

lib/iris/coord_systems.py Outdated

Copy link

Member

rhattersley Aug 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you add "Defaults to 0." for these arguments.

lbdreyer force-pushed the lambert-equal branch 3 times, most recently from 951b4a3 to 5441678 Compare September 28, 2016 13:09

lbdreyer commented Sep 28, 2016

View reviewed changes

bjlittle added the Status: Work in Progress label Sep 28, 2016

lbdreyer mentioned this pull request Sep 28, 2016

Update version number on master #2153

Merged

lbdreyer force-pushed the lambert-equal branch from 5441678 to 0af35ac Compare September 30, 2016 14:30

lbdreyer commented Sep 30, 2016

View reviewed changes

lbdreyer force-pushed the lambert-equal branch 3 times, most recently from 35883f7 to 9e3a3b8 Compare October 3, 2016 13:49

laea

cde0ac4

lbdreyer force-pushed the lambert-equal branch from 9e3a3b8 to cde0ac4 Compare October 10, 2016 09:14

bjlittle requested changes Oct 10, 2016

View reviewed changes

Update laea test; remove rotated cs changes

fa72ee7

lbdreyer force-pushed the lambert-equal branch from 5bbe26e to fa72ee7 Compare October 10, 2016 14:33

bjlittle approved these changes Oct 10, 2016

View reviewed changes

bjlittle merged commit c577f76 into SciTools:master Oct 10, 2016

bjlittle removed the Status: Work in Progress label Oct 10, 2016

QuLogic added this to the v1.11 milestone Oct 11, 2016

djkirkham mentioned this pull request Oct 24, 2017

Remove decoupler branch #2851

Closed

lbdreyer deleted the lambert-equal branch July 23, 2018 10:48

Conversation

lbdreyer commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhattersley commented Aug 5, 2015

Uh oh!

lbdreyer commented Aug 5, 2015

Uh oh!

rhattersley commented Aug 5, 2015

Uh oh!

lbdreyer commented Sep 28, 2016

Uh oh!

lbdreyer Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbdreyer commented Sep 28, 2016

Uh oh!

ajdawson commented Sep 28, 2016

Uh oh!

lbdreyer commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbdreyer commented Oct 3, 2016

Uh oh!

marqh commented Oct 10, 2016

Uh oh!

lbdreyer commented Oct 10, 2016

Uh oh!

bjlittle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle commented Oct 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lbdreyer Sep 28, 2016 •

edited

Loading

lbdreyer commented Sep 30, 2016 •

edited

Loading